CUST-4514 added handling for non-json responses and tests#424
CUST-4514 added handling for non-json responses and tests#424samuelpx wants to merge 12 commits into
Conversation
…es-html-fastly-empty-etc-stemming-from-500-s
…ained backwards compatibility
…es-html-fastly-empty-etc-stemming-from-500-s
…ained backwards compatibility
| """ | ||
|
|
||
| request_id: str | ||
| request_id: Optional[str] |
There was a problem hiding this comment.
I don't believe this is ever optional for our API effors
There was a problem hiding this comment.
Thanks, I'll correct this!
| self.headers: CaseInsensitiveDict = headers | ||
|
|
||
|
|
||
| class NylasNetworkError(AbstractNylasSdkError): |
There was a problem hiding this comment.
Where are we actually utilizing this new error?
There was a problem hiding this comment.
Nowhere yet, the idea is that it will replace the proposed non-json handling implementation in the future, we currently raise a NylasApiError to keep backwards compatibility as a priority, let me know if we should just use this instead.
…rmation to non-json error
Pull Request Summary
Functional Tests
Files Changed
View more in PlayerZero |
…es-html-fastly-empty-etc-stemming-from-500-s
…es-html-fastly-empty-etc-stemming-from-500-s
nylas + PlayerZeroSimulations
View more in PlayerZero |
…es-html-fastly-empty-etc-stemming-from-500-s
AaronDDM
left a comment
AaronDDM
left a comment
There was a problem hiding this comment.
Solid direction — handling non-JSON 4xx/5xx responses fixes a real pain point. A few things to address before merging:
- CHANGELOG entry is under
v6.10.0(already released). Move it to anUnreleasedsection so it shows up in the next release notes. - Multi-line f-string in the error message produces ugly literal indentation (24 spaces of leading whitespace on each subsequent line). See inline comment.
NylasNetworkErroris declared but never raised anywhere. Either wire it up or remove it; dead code is confusing (this was flagged back in June and is still unresolved).AbstractNylasSdkErrorfields made Optional — already flagged in the earlier review.request_id/status_code/headersare not optional for genuine API errors; weakening these types just to satisfy the network-error path regresses type safety for the main flow. Prefer keeping them required onAbstractNylasSdkErrorand letNylasNetworkError(once wired up) override with Optional.- Tests assert exact whole-message strings, which couples them to the indentation-bug formatting. Once #2 is fixed every test will break; assert on substrings (status code,
flow_id, body excerpt) instead. - Silent
return ({}, response.headers)on non-JSON 2xx — minor, but worth a debug log so this doesn't silently mask a malformed-but-successful response.
|
|
||
| v6.10.0 | ||
| ---------------- | ||
| * Added handling for non-JSON responses |
There was a problem hiding this comment.
This entry is under v6.10.0, which has already been released. Please move it under an Unreleased section at the top so it lands in the next release notes.
| type="network_error", | ||
| message=f""" | ||
| HTTP {response.status_code}: Non-JSON response received{flow_info}. | ||
| Body: {body_preview}""", |
There was a problem hiding this comment.
The triple-quoted f-string inlines the source indentation into the error message, so the rendered text looks like:
HTTP 500: Non-JSON response received (flow_id: ...).
Body: ...
That'll look awful in logs and stack traces. Either collapse to a single line:
message=f"HTTP {response.status_code}: Non-JSON response received{flow_info}. Body: {body_preview}"or use textwrap.dedent + .strip(). The existing tests assert on the indented string, so they'll need updating too — ideally to substring assertions.
| status_code=response.status_code, | ||
| headers=response.headers, | ||
| ) from exc | ||
| return ({}, response.headers) |
There was a problem hiding this comment.
Silently returning ({}, headers) for a 2xx non-JSON body could mask a real problem (e.g. a malformed JSON body that happens to come back with 200). At minimum add a debug log; better, consider raising for unexpected non-JSON success too, since every JSON endpoint in this SDK should return parseable JSON.
| self.headers: CaseInsensitiveDict = headers | ||
| self.request_id: Optional[str] = request_id | ||
| self.status_code: Optional[int] = status_code | ||
| self.headers: Optional[CaseInsensitiveDict] = headers |
There was a problem hiding this comment.
Per the earlier review thread — these shouldn't become Optional on the base class. Genuine API errors always have a status_code and headers. Keep them required here and let NylasNetworkError declare its own Optionals if needed.
| self.status_code: Optional[int] = status_code | ||
| self.raw_body: Optional[str] = raw_body | ||
| self.headers: Optional[CaseInsensitiveDict] = headers | ||
| self.flow_id: Optional[str] = flow_id |
There was a problem hiding this comment.
This class is defined but never instantiated. Either raise it from _validate_response instead of (or alongside) NylasApiError, or drop it from this PR and add it when the v7.0 work lands. Right now it's just dead code and a maintenance trap.
| response.headers = {"Content-Type": "text/html", "x-fastly-id": "fastly-123"} | ||
|
|
||
| with pytest.raises(NylasApiError) as e: | ||
| _validate_response(response) |
There was a problem hiding this comment.
These assert str(e.value) == """...""" checks pin the exact whitespace of the error string and will all break once the multi-line f-string is fixed. Recommend asserting on substrings, e.g.:
assert "HTTP 500" in str(e.value)
assert "flow_id: fastly-123" in str(e.value)
assert "<h1>Internal Server Error</h1>" in str(e.value)2 participants
License
I confirm that this contribution is made under the terms of the MIT license and that I have the authority necessary to make this contribution on behalf of its copyright owner.